Add 'compare performance' view #3843

asgerf · 2024-11-27T10:34:35Z

Adds the 'Compare Performance' UI from the hackathon.

Activate via right-clicking on a query history item

Shows overview of predicate evaluation costs

Click a predicate to show its pipeline(s) and tuple counts per step

Changes since the hackathon:

The integration with DCA has been left out for now. I'm hoping @esbena will follow up on this.
Added support for predicate-renaming, which was WIP and not demoable in time for the Hackathon demo.
Some UI performance improvements: using useMemo and moved some state into subcomponents where a state change cause as much re-rendering.
Some clean up of the commit history; mainly squashing simple bugfixes into an earlier commit.

Despite my attempts at cleaning the commit history, it might actually be easiest to review as one giant diff.

Adds a dropdown with (at present) two options: sorting by delta and sorting by absolute delta.

This ensures we can use hooks after the check in the main component

A very hacky implementation that simply instantiates an empty `PerformanceOverviewScanner` as the "from" column (i.e. with all values empty). To see it in action, select a single query run in the query history and pick "Compare Performance" from the context menu. Then select the "Single run" option when prompted.

Only contains formatting changes

…eMetric

Predicate names can't be empty, but it can happen with the renaming feature added in the next commit.

aeisenberg

It looks ok to me in general. I haven't tried running this. It also seems isolated, so that if there is a problem, it is unlikely to affect other features.

There are a few questions and comments inside. Mostly, I'd like to either remove all TODOs and XXXs and replace them with a simple comment. If you think you will go back later to fix them, then please create an issue to track it.

extensions/ql-vscode/package.json

aeisenberg · 2025-01-10T21:25:37Z

extensions/ql-vscode/src/compare-performance/compare-performance-view.ts

+    };
+  }
+
+  protected onPanelDispose(): void {}


Do you want to remove references to resultsView and labelProvider?

Suggested change

protected onPanelDispose(): void {}

protected onPanelDispose(): void {

this.resultsView = null;

this.labelProvider = null;

}

Hmmm...not sure what the lifecycle of the view is. Is there a single view instance created for the entire extension, or is it one view per window that's opened?

The instance of this class persists across multiple open/close of the panel so I'd say we shouldn't dispose of these fields.

I see how onPanelDispose is called in the super class. Your implementation here is fine.

extensions/ql-vscode/src/log-insights/log-scanner.ts

aeisenberg · 2025-01-10T21:33:25Z

extensions/ql-vscode/src/log-insights/performance-comparison.ts

+ */
+export interface PerformanceComparisonDataFromLog {
+  /** Names of predicates mentioned in the log */
+  names: string[];


Do we expect the lengths of these arrays to be the same for all? If not, which ones will have different sizes?

Added a more thorough description

aeisenberg · 2025-01-10T21:34:39Z

extensions/ql-vscode/src/log-insights/performance-comparison.ts

+  /**
+   * List of indices into the `names` array for which we have seen a cache hit.
+   *
+   * TODO: only count cache hits prior to first evaluation?


Here and below, if the TODO is tracked in an issue, can you add the issue? If it's not tracked, can you remove the TODO?

I'll remove or resolve the TODOs 👍

For this one I went ahead and resolved it

aeisenberg · 2025-01-10T21:37:35Z

extensions/ql-vscode/src/query-history/query-history-manager.ts

+    singleItem: QueryHistoryInfo,
+    multiSelect: QueryHistoryInfo[] | undefined,
+  ) {
+    // TODO: reduce duplication with 'handleCompareWith'


Same...and below.

aeisenberg · 2025-01-10T21:38:15Z

extensions/ql-vscode/src/query-history/query-history-manager.ts

+    multiSelect: QueryHistoryInfo[] | undefined,
+  ) {
+    // TODO: reduce duplication with 'handleCompareWith'
+    multiSelect ||= [singleItem];


That's a nice shortcut. I hadn't thought of that.

aeisenberg · 2025-01-10T21:39:31Z

extensions/ql-vscode/src/query-history/query-history-manager.ts

+          ? allSelectedItems[1]
+          : allSelectedItems[0];
+      if (otherItem.databaseName !== dbName) {
+        throw new Error("Query databases must be the same.");


Minor: can you include the db names in this message?

I don't remember why this condition is even here; it's possible we should just remove the restriction. I've asked @tausbn if he remembers why it's here

I would be fine with removing this. I have no recollection of why it would be necessary, and I don't think anything would blow up if you compare two different databases. At worst, you just get somewhat useless output.

It seems this snippet of code was copy/pasted from findOtherQueryToCompare but isn't actually relevant for performance comparisons. I pushed a commit that removes the unnecessary check.

aeisenberg · 2025-01-10T21:50:58Z

extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx

+        args.push(parseQName());
+        skipToken(",");
+      }
+      args.reverse();


Why the reverse?

Added a comment to clarify

Note that the tokens stream is parsed in reverse order. This is simpler, but may look confusing initially.

aeisenberg · 2025-01-10T21:58:36Z

extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx

+        if (prefix != null) {
+          result.push(prefix.abbreviate());
+          result.push("::");
+        }


This is quite complex to me. It is combining prefix from the closure with result, a local variable. This function is complex enough that maybe VisitResult should be a class and prefix is passed in as a private field. Then this function can become a member of VisitResult.

I guess this is a matter of taste. Personally I find captured variables to be simpler to follow than fields as you can just jump to it to see what it is. This actually started out as a class, which I converted to a closure to simplify it. Do you feel strongly about doing this rewrite?

This is fine by me. I can understand your perspective even if it is not how I would have written it. And it doesn't seem incorrect to me.

Restrict to Canary Co-authored-by: Andrew Eisenberg <[email protected]>

Co-authored-by: Andrew Eisenberg <[email protected]>

…odeql into asgerf/compare-perf-view

The snippet seems to have been copied from 'findOtherQueryToCompare' where it makes sense, but in the context of a performance comparison we don't need this restriction.

asgerf · 2025-01-17T11:53:18Z

Thanks for the review @aeisenberg! I've responded to all comments

aeisenberg

LGTM. Thanks for addressing my comments.

After you merge, you can do a release. If you don't feel comfortable doing a release, then send a message to the internal slack channel and we can help you.

aeisenberg · 2025-01-17T23:28:54Z

extensions/ql-vscode/src/compare-performance/compare-performance-view.ts

+    };
+  }
+
+  protected onPanelDispose(): void {}


I see how onPanelDispose is called in the super class. Your implementation here is fine.

aeisenberg · 2025-01-17T23:30:35Z

extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx

+        if (prefix != null) {
+          result.push(prefix.abbreviate());
+          result.push("::");
+        }


This is fine by me. I can understand your perspective even if it is not how I would have written it. And it doesn't seem incorrect to me.

asgerf · 2025-01-22T10:03:05Z

Thanks for the review! I've merged for now, and will see if I can do a release.

asgerf and others added 30 commits November 27, 2024 13:47

Add Compare Performance command (WIP)

3b06977

Show evaluation and iteration counts in table

96aa770

compare-perf: Add support for sorting options

aaf23ea

Adds a dropdown with (at present) two options: sorting by delta and sorting by absolute delta.

Make RAPrettyPrinter generate JSX fragments

70ec570

Apply styling to RA predicate names

8268d68

Apply a background color to the pipeline header rows

58afeba

Make "..." clickable to reveal abbreviated name

03ca407

Also abbreviate RA names in predicate overview

317e52c

Colorize positive/negative deltas

876c5b6

Add option to choose metric

260bf0e

Check for nullness of 'data' in a separate component

453aa83

This ensures we can use hooks after the check in the main component

Simplify datasets assignment

ccf2dc6

feat: parallel log scanning

412338c

Simplify view when not in comparison mode

e039f6b

Allow word wrap to break anywhere

1d2c2cf

Use useMemo for 'nameSet'

b05ec33

Use useMemo a few places to speed up UI interactions

20f6e3d

Reformat code

558d957

Also useMemo the 'total' row computation

6568b56

Reset hasCacheMismatch when rebuilding 'rows'

6f7eb74

Reformat code again

62f3b4f

Only contains formatting changes

Use interface instead of type alias for TRow

9800fa1

Refactor OptionalValue

d008963

Split renderAbsoluteValue into renderOptionalValue and renderPredicat…

eec42c5

…eMetric

Refactor predicate row into a separate component

9a0699f

Rename TRow -> Row

48954c7

Group together rows by fingerprinting

ab00152

Factor out rendering of table body to a memoized component

4a835b8

Fix some crashes when pretty-printing an empty name

568f082

Predicate names can't be empty, but it can happen with the renaming feature added in the next commit.

asgerf force-pushed the asgerf/compare-perf-view branch 2 times, most recently from 13e78a6 to ec223fa Compare November 27, 2024 13:00

asgerf added 7 commits November 27, 2024 14:14

Add predicate-renaming support

2cae71c

Add 'per evaluation' option next to metric

d37469f

Move "total" row to the top and render the metric

8a58279

Hide "sort by" dropdown when there is no delta to sort by

b9d1551

Factor header rows into a component

afa3d55

Only warn about cache hits in comparison mode

c99bf5b

Remove unused export

aa528c6

asgerf force-pushed the asgerf/compare-perf-view branch from ec223fa to aa528c6 Compare November 27, 2024 13:14

asgerf marked this pull request as ready for review November 27, 2024 13:41

asgerf requested a review from a team as a code owner November 27, 2024 13:41

aeisenberg reviewed Jan 10, 2025

View reviewed changes

asgerf and others added 10 commits January 17, 2025 10:40

Update extensions/ql-vscode/package.json

6f461e7

Restrict to Canary Co-authored-by: Andrew Eisenberg <[email protected]>

Update extensions/ql-vscode/src/log-insights/log-scanner.ts

2293cc3

Co-authored-by: Andrew Eisenberg <[email protected]>

Add more description of the "struct of arrays" layout

08bffab

Merge branch 'asgerf/compare-perf-view' of github.com:asgerf/vscode-c…

44d33d6

…odeql into asgerf/compare-perf-view

Only record cache hits prior to first evaluation

f09210b

Remove TODO that was just resolved

37dcd08

Remove TODOs

370b17c

Clean up some more TODOs

8b8d174

Add comment to clarify reverse parsing

bba31c0

Permit performance comparisons across DBs

666c26e

The snippet seems to have been copied from 'findOtherQueryToCompare' where it makes sense, but in the context of a performance comparison we don't need this restriction.

aeisenberg approved these changes Jan 17, 2025

View reviewed changes

asgerf merged commit ffed4b6 into github:main Jan 22, 2025
16 checks passed

robertbrignull mentioned this pull request Jan 23, 2025

Release v1.17.1 #3908

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'compare performance' view #3843

Add 'compare performance' view #3843

asgerf commented Nov 27, 2024 •

edited

Loading

aeisenberg left a comment

aeisenberg Jan 10, 2025

aeisenberg Jan 10, 2025

asgerf Jan 17, 2025 •

edited

Loading

aeisenberg Jan 17, 2025

aeisenberg Jan 10, 2025

asgerf Jan 17, 2025

aeisenberg Jan 10, 2025

asgerf Jan 17, 2025

aeisenberg Jan 10, 2025

aeisenberg Jan 10, 2025

aeisenberg Jan 10, 2025

asgerf Jan 17, 2025 •

edited

Loading

tausbn Jan 17, 2025

asgerf Jan 17, 2025

aeisenberg Jan 10, 2025

asgerf Jan 17, 2025

aeisenberg Jan 10, 2025

asgerf Jan 17, 2025

aeisenberg Jan 17, 2025

asgerf commented Jan 17, 2025

aeisenberg left a comment

aeisenberg Jan 17, 2025

aeisenberg Jan 17, 2025

asgerf commented Jan 22, 2025

Add 'compare performance' view #3843

Add 'compare performance' view #3843

Conversation

asgerf commented Nov 27, 2024 • edited Loading

Activate via right-clicking on a query history item

Shows overview of predicate evaluation costs

Click a predicate to show its pipeline(s) and tuple counts per step

Changes since the hackathon:

aeisenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asgerf Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asgerf Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asgerf commented Jan 17, 2025

aeisenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asgerf commented Jan 22, 2025

asgerf commented Nov 27, 2024 •

edited

Loading

asgerf Jan 17, 2025 •

edited

Loading

asgerf Jan 17, 2025 •

edited

Loading